Skip to content

fork: green main again (schema-check comments, checkout without submodule cleanup) - #8

Merged
scuttletoby merged 2 commits into
mainfrom
fork/fix-main-lint-and-schema-checkout
Nov 12, 2026
Merged

fork: green main again (schema-check comments, checkout without submodule cleanup)#8
scuttletoby merged 2 commits into
mainfrom
fork/fix-main-lint-and-schema-checkout

Conversation

@scuttletoby

@scuttletoby scuttletoby commented Nov 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Main (ae0d686) is red on two jobs, so every fork PR is red. This fixes both.

1. Lint: t3code/no-low-value-comments (8 errors)

All eight were comments in .github/scripts/fork-schema-check.cjs (from #3) over the four-line cap. I cut them to the constraint they carry (why barrels are guarded but not followed, why other export entries and dependency specs stay free, why absent/non-JSON manifests get their own value, the file header's list of what is refused). I dropped the parts that restate the code and the long JSDoc type blocks, since the file is not type-checked. The rule is not disabled. vp lint and vp fmt --check are clean on the three files.

2. "No database schema changes": checkout died before any script ran

Exact error (PR #4's run 43226799575, head c861d84), in actions/checkout's own step, under "Removing auth":

git submodule foreach --recursive sh -c "git config --local ... core.sshCommand ..."
fatal: No url found for submodule path '.repos/alchemy-effect/submodules/distilled' in .gitmodules

persist-credentials: false makes actions/checkout remove auth right after checkout, and that runs git submodule foreach. Upstream's tree has two gitlinks under .repos/alchemy-effect/submodules/ and no .gitmodules. submodules: false is already the default and does not help. A sparse checkout does not help either, because submodule foreach reads gitlinks from the index (checked locally).

Fix: the job uses plain git and no checkout action. It runs git init, then one anonymous --filter=blob:none --no-recurse-submodules fetch of github.sha (the default branch under pull_request_target), base and head. Then a full checkout of github.sha. A full checkout fetches main's blobs in one batch. A sparse one left the check to fetch each blob it reads separately (64s instead of 1s on the proof run). Git never runs a submodule command.

  • Still pull_request_target, and still no checkout of PR code: the head is only fetched as objects.
  • No token: the repository is public and nothing is persisted.
  • Event values reach the shell only through env:.
  • Fail closed: set -euo pipefail, so any fetch or checkout error is red. The test covers a commit the remote lacks.

Tests

  • e2e: a tree with a gitlink and no .gitmodules (unchanged, and bumped by the PR) is checked like any other tree and passes.
  • e2e: the workflow's own checkout step, pulled from fork-checks.yml and run with bash -eo pipefail the way Actions does, against a local origin with a gitlink and no .gitmodules. It asserts:
    • the step exits 0;
    • HEAD is the default branch, not the PR;
    • the PR head is present as objects;
    • an unknown head SHA fails the step.
      Mutation check: adding git submodule foreach --recursive true to the step makes this test fail with the exact CI error.
  • Workflow shape test, tightened: no actions/checkout, ref:, secrets. or GITHUB_TOKEN, and the only git checkout|switch|worktree|reset|read-tree is git checkout -q --detach "$DEFAULT_SHA" with DEFAULT_SHA: ${{ github.sha }}. The pin test now allows a workflow that uses no actions.
  • node --test .github/scripts/fork-schema-check.test.cjs: 49/49.

About this PR's own "No database schema changes" run

It will stay red, for two reasons:

  1. pull_request_target runs main's copy of fork-checks.yml, which still has the broken checkout.
  2. Even with the new workflow, this PR edits the check's own files. The check refuses that by design once the base has the check ("Only the owner may do that"), so it needs the owner's override.

The new job is proven separately on a throwaway proof PR. It runs this branch's workflow under pull_request against a base that already has the new check. The links are in a comment below.

State: what happens when this runs again, stops half-way, or meets existing/old/empty state?

  • Re-run: each run starts in a fresh runner workspace, does git init and fetches by SHA. Nothing carries over, so a re-run does the same thing.
  • Stops half-way: a failed fetch or checkout ends the step non-zero, and the job is red. The check script only runs after a full checkout of main. There is no state outside the runner, so nothing is half-applied.
  • PR on old main / stale base: the fetch takes base and head with full history (no depth limit), so the merge-base and the stale-commit detection still work. A head the remote does not have fails the job.
  • Empty diff: the script reports "0 changed files checked" and passes, as before. Verified locally against a fork copy with the real gitlinks.
  • Legacy trees with gitlinks / no .gitmodules: handled. ls-tree and diff already treat mode-160000 entries as "no blob", and the new e2e test covers it.
  • Base without the check (the series that first adds it): unchanged, covered by the existing e2e test.

Not covered here

fork-release.yml uses actions/checkout with persist-credentials: false in three jobs (lines ~99, ~139, ~354), so the same "Removing auth" failure probably hits those on upstream's tree. I did not change or run them.

🤖 Generated with Claude Code

@scuttletoby @claude
fork: green main again (schema-check comments, checkout without submo…
936e8c7 
…dule cleanup)

Lint: eight comments in fork-schema-check.cjs were over the four-line cap of
t3code/no-low-value-comments. Cut to the constraints; the restating parts and
JSDoc type blocks are gone (the file is not type-checked).

No database schema changes: actions/checkout with persist-credentials: false
runs `git submodule foreach` to remove auth, and upstream's tree has gitlinks
under .repos/alchemy-effect/submodules/ with no .gitmodules, so it died with
"No url found for submodule path" before any script ran. The job now uses
plain git: init, an anonymous blob:none fetch of the default branch, base and
head, a sparse checkout of .github at github.sha. Still pull_request_target,
still no checkout of PR code, any git error fails the step.

Tests: a gitlink with no .gitmodules in the e2e set, and the workflow's own
checkout step run against such a repository (plus fail-closed on a missing
commit). The workflow shape test now allows only a checkout of github.sha.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@scuttletoby scuttletoby mentioned this pull request Nov 12, 2026
Closed
@scuttletoby @claude
fork-checks: full checkout of the default branch, so the check does n…
e76c877 
…ot fetch blobs one by one

A sparse checkout left every blob the check reads to a separate lazy fetch
from the partial clone (about 170 round trips on the real tree), close to the
job's five-minute timeout. A full checkout fetches the default branch's blobs
in one batch, as actions/checkout did before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@scuttletoby

scuttletoby commented Nov 12, 2026

Copy link
Copy Markdown
Owner Author

Proof for the new fork-checks.yml (pull_request_target on this PR still runs main's broken copy, so that check stays red here by design):

@scuttletoby

scuttletoby commented Nov 12, 2026

Copy link
Copy Markdown
Owner Author

lens: correctness+security · reviewer: opus high (t3lens-8) · read at: e76c877

covered: full diff origin/main...e76c877 (fork-checks.yml, fork-schema-check.cjs, fork-schema-check.test.cjs), plus what the check calls into (git(), readTree, readBlob, blobAt, staleCommits, main). Compared with upstream's own plain-git checkout in desktop-macos-preview.yml, the proof run 43229342558 (its workflow is byte-identical to this head's except for the trigger, and every step is green; the checkout took 21s), the failing run 43230803534 (main's copy, "No url found for submodule path"), and every actions/checkout use on fork main.

My view first: a pull_request_target job that swaps actions/checkout for plain git must guarantee five things. (a) The work tree, and every script that runs, comes from the commit whose workflow is running (github.sha). (b) The PR head only arrives as objects. (c) No credential reaches disk or the remote URL. (d) Event values reach bash only through env. (e) Every fetch or checkout failure exits non-zero. The step does all five:

  • git init, an anonymous remote "$GITHUB_SERVER_URL/$GITHUB_REPOSITORY", and one fetch of the three SHAs by id (no refs, no tags, no depth limit, blob:none, --no-recurse-submodules). Then checkout --detach "$DEFAULT_SHA". Nothing runs a submodule command.
  • The check reads base, head and merge-base only by SHA (ls-tree, cat-file, merge-base, diff, log), so it sees the same files as before. It never uses a ref that actions/checkout used to create (refs/remotes/*, tags).
  • The .cjs diff touches comments only: every +/- line is a // or JSDoc line. The trims keep every reason a reader needs. The schema-statement scan is still documented at :26, the unresolved/outside rule in traceSchema's block, and the absent/non-JSON rule in manifestMapping's. What went is either restated code (the fallback field list, which sits at :476/:531) or JSDoc type blocks.

checks:

  • At the head, node --test fork-schema-check.test.cjs passes 49/49 (git 2.43, node 24).
  • Mutations, run one at a time and restored after each:
    • git submodule foreach --recursive true put back: the e2e checkout test goes red.
    • Checking out "$HEAD_SHA" instead: the shape test and the e2e go red.
    • || true after the fetch: the e2e goes red (the unknown-SHA case).
    • A token in the remote URL via ${{ github.token }}: red.
    • Dropping set -euo pipefail: stays green. Actions' default shell is already bash -e -o pipefail and the harness runs the same way, so that is not a gap.
    • --depth=1: the tests stay green, but I ran the check by hand on a depth-1 fetch and git merge-base fails, so the job exits 1 (red, never a wrong green).
    • Two variants stay green: findings 1 and 2.
  • Fail closed: if one wanted SHA is missing, the whole fetch fails (the e2e covers it). A force-pushed head that GitHub has gc'd fails the fetch. A missing merge-base or blob makes execFileSync throw, so node exits non-zero. A private repo, or one where anonymous fetch is refused, also fails red.
  • State question ("runs again, stops half-way, old/empty state"):
    • Re-run: same event payload, same SHAs, and a fresh hosted workspace, so the result is the same. A re-run of an event whose head was force-pushed and gc'd stays red until the next push.
    • Stops half-way: no state lives outside the runner, and each step needs the one before it, so a half-done job is a red job.
    • Leftover workspace: on a reused or self-hosted workspace, git remote add origin fails, which is red and never stale data. ubuntu-latest is always fresh.
    • PR on old main: full history, so merge-base and staleCommits still work. Head-only blobs are still lazy-fetched one by one, as before this PR. A very stale PR could be slow, and timeout-minutes 5 then turns it red, never green.
    • Empty diff: "0 changed files checked", green, as before.
    • A PR that deletes or edits fork-checks.yml: main's copy still runs (pull_request_target), and CHECK_FILES (:19-23) refuses the edit.
    • Trees with gitlinks and no .gitmodules: covered by the new e2e, including a bumped gitlink.
  • Other workflows on fork main: only Fork checks, Fork tests and Fork release are active; upstream's are disabled_manually. Fork tests' 12 checkouts use the default persist-credentials and were green on main 82bebcb, so the failure needs persist-credentials: false. Fork release is active, has never run, and has four jobs with persist-credentials: false (finding 3).
  • Not counted: Test Web 2 (known flake) and this PR's own pull_request_target run (main's broken copy, plus the check refusing its own files by design). Lint is green on Fork tests 43229334351.

finding: minor .github/scripts/fork-schema-check.test.cjs:463 The "event values only via env" guard only scans lines that contain run:. The checkout step is now a run: | block, so its body is never scanned. Adding echo '${{ github.event.pull_request.title }}' >/dev/null inside that block leaves all 49 tests green, and on pull_request_target that is script injection. The step as written is clean; this is a test regression the block scalar brings. Fix: scan every line of each run block, or every line outside env:, for ${{. (high)
finding: minor .github/scripts/fork-schema-check.test.cjs:457 The token ban covers secrets. and GITHUB_TOKEN but not github.token. Adding AUTH: ${{ github.token }} in env and git config http.extraheader "AUTHORIZATION: bearer ${AUTH:-}" after git init writes the token into .git/config, and all 49 tests stay green. That breaks the PR's "No token" claim with no test going red. Fix: add github\.token to that regex. (high)
finding: minor .github/workflows/fork-release.yml:99,139,354,473 It is four jobs, not the three the PR body names. publish (:473) uses a sparse checkout, which by this PR's own finding does not avoid the gitlink failure. Fork release is active and has never run, so the first release will most likely stop at checkout. It fails red, not silently, and it is out of scope here; it needs its own issue or PR. (medium, not run)

@scuttletoby
scuttletoby merged commit 36d66b8 into main Nov 12, 2026
38 of 44 checks passed
This was referenced Nov 12, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@scuttletoby